Make Base.get_extension private to prevent user mistakes
#59708
Conversation
Added in #58108 without valid justification: something simply being documented in the manual is never any allowable argument for making something public. Being public is confusing users, since all of the other related infrastructure is also declared private (project files, `loaded_modules_array`, `PkgId`, `require`, etc.). Some of those do have legit uses in reflection testing, but we don't currently have a representation for this sort of private-except-reflection in most places (other than `Core.IR`). However is not allowed for packages to call `Base.get_extension`, since it requires accessing private implementation details of the target package to construct the arguments, and can run afoul of various implementation rules (world ages, module identity, precompile file consistency) that can lead to buggy execution. The correct way to use this functionality is with dispatch. Loosely: ```julia module Main function get_extension end end module MainPkgExt # extension module in Main using Other Main.get_extension(args...) = Other.call(args...) end ```
is this referring to the name of the extension? (surely the package's module itself is not private?) |
giordano
added
the
needs pkgeval
|
There are hundreds of packages using this already: https://juliahub.com/ui/Search?type=code&q=Base.get_extension |
KristofferC
removed
the
backport 1.12
|
Docstring can be updated to avoid user mistakes. |
|
That's fine. I am not proposing that we delete it, simply to communicate formally (to lint tools) that this is not an approved way to do things. There is no pkgeval required, since this annotation does not affect runtime. |
vtjnash
removed
the
needs pkgeval
Yes, I didn't not write reland or revert since this is the requested doc change. |
| Return the module for `extension` of `parent` or return `nothing` if the extension is not loaded. | ||
| Return the module for `extension` relative to `parent` or return `nothing` if the extension is not loaded. | ||
| This function is private, since the arguments to it are private implementation details. |
There was a problem hiding this comment.
In how far are the arguments private?
There was a problem hiding this comment.
The ext name is private, and the parent argument is used to extract PkgId which is internally also private
A module is private, other than its own name & the things it declares public internally: a module reference does not give you the public permission to do any arbitrary reflection with it (otherwise the whole public/private distinction is moot). This is a reflection function, and thus simply having the module reference does not imply you can call this function. |
|
It's up to package authors to decide whether they consider extension module names public or not. For packages that do, Also, this function can totally legitimately be used in the package defining the extension itself. |
|
They cannot, since this is private to Base. Particular not in the package itself, since that means the package loads its own extension, which will trigger at lot of issues |
|
Alright, we accidentally made a release with this marked public, so now we have a public API that is forbidden for packages to actually use, since actual use may corrupt their precompile files or crash their users :/ |
|
(Bugfix in the sense that adding the |
|
Note that |
Not sure what exactly this is in response to – to this?
|
Just be super clear again: being documented never makes something public, unless it is separately declared public. This rule is to prevent something from going undocumented simply because it is only for private use. |
|
Being documented in the manual has always been the rule for what's public in Julia, until very recently (only changed in the last version, 1.11). |
I assume you mean recommend against? The Pkg docs mention get_extension in the context of the following sentence: "On the other hand, accessing extension symbols from a third-party package (i.e. not the parent) is not a recommended practice at the moment." |
There's a couple of sentences just above that, including: It's a pretty strong recommendation, using "must". |
Also merged at the same time (these were both added in v1.7) was a statement in the manual that non-exported functions may be mentioned in the manual are still usually private unless the documentation states otherwise (e.g. by adding export or stating it is the API in the documentation): |
|
I am a little confused by what the Pkg.jl authors had in mind, since normally a package wouldn't use its own extension, since why would it be an extension otherwise (it creates a dependency cycle and can create ordering problems with precompilation) |
Note that's the style guide for writing code in Julia :) |
There are quite a few reasonable and legit usages, checking @giordano's link with hundreds of packages using |
|
Yes, that is what I'm referring to as being known to be fairly buggy and crashy because it causes cycles in the loading resolution code (e.g. the partial fix at #55589, and the many linked issues there, particularly #56204 (comment)) and leads to incorrectly generated cache files. As @KristofferC said originally "In my view, this should mostly be used as a debugging tool and should not be something that is part of the public API of a package " 😭 #47982 (comment) |
|
I need to "update" the branch to fix the trimming failure. |
DilumAluthge
removed
the
merge me
|
(Removing |
Added in #58108 without valid justification: something simply being documented in the manual is never any allowable argument for making something public. Being public is confusing users, since all of the other related infrastructure is also declared private (project files,
loaded_modules_array,PkgId,require, etc.). Some of those do have legit uses in reflection testing, but we don't currently have a representation for this sort of private-except-reflection in most places (other thanCore.IR).However is not allowed for packages to call
Base.get_extension, since it requires accessing private implementation details of the target package to construct the arguments, and can run afoul of various implementation rules (world ages, module identity, precompile file consistency) that can lead to buggy execution.The correct way to use this functionality is with dispatch. Loosely: